-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: persistent errors for job processing [DHIS2-15276] #15575
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15575 +/- ##
============================================
- Coverage 66.23% 66.19% -0.04%
+ Complexity 31250 31240 -10
============================================
Files 3485 3485
Lines 129791 129851 +60
Branches 15145 15165 +20
============================================
- Hits 85965 85960 -5
- Misses 36741 36802 +61
- Partials 7085 7089 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…UID [DHIS2-15276]
Kudos, SonarCloud Quality Gate passed! |
} | ||
|
||
default void addError(ErrorCode code, String uid, String type, Integer index, List<String> args) { | ||
// default implementation is a NOOP, we don't remember or handle the error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow this reasoning for this empty method.
Do only certain jobs care about errors & they would then override this method? (which would result in the persisting of the error(s)?
And if jobs using this default method have errors, they are silently swallowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to certain jobs but only one implementation of the JobProgress
interfaces wants to record them and here we have no access to the implementation. To avoid having to implement the method in all other implementation with an empty method it is instead implemented like that as default
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the default to be an empty method suggests it shouldn't be part of the interface. It feels like we're adding this confusing behaviour to satisfy only 1 implementer.
Could a javadoc comment be added to the method at least to explain why we don't want to handle errors here (handling errors seems like a very common use case) as the default and when would overriding be necessary. This would at least help someone reading the code to hopefully understand the reasoning for it & when it should be overridden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that in the next PR I was planning on doing anyhow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch with the jsonb
Summary
Main changes to extend the job progress tracking API with a error collection feature.
Errors are attached to the overall
progress
JSON object.A new column
errorCodes
was added to the table which contains the codes extracted from theprogress
object.This is purely to allow easier search by error code(s) using existing filter API tech. The column is implicitly maintained when the progress JSON changes.
To accommodate this the persisted JSON changed from an array of processes which previously would be wrapped in a
Progress
object on in the memory model which had thesequence
ofProcess
es this root object is now explicit in the stored JSON so it can also contain theerrors
list. To be backwards compatibly with the JSON array stored previously the extraction pf the progress JSON will auto-wrap any array values in an appropiate object in the SQL so existing data will continue to work.Other changes
ErrorReport
of the metadata import now does contain the plain arguments to the message so they can be forwarded to the job progress error listError
class is now calledTrackerImportError
in OpenAPI to resolve the name clash with the new genericError
class addedALL
orF_PERFORM_MAINTENANCE
authority or must be performed by the user executing the job/progress
and/errors
is restricted toALL
orF_PERFORM_MAINTENANCE
authorityFixes
progress
JSON object is now stored as ajsonb
object not asstring
of the escaped JSON (that worked by accident for store and decode)Automatic Testing
The affected code is covered by many existing test indirectly. No explicit new tests were added yet.
Manual Testing
/api/jobConfigurations/{uid}/errors
and/api/jobConfigurations/{uid}/progress
endpoints of the job created for the import and check the error added to the file is recorded (find the UID e.g. by the newestMETADATA_IMPORT
job in the listapi/jobConfigurations/gist?order=created:desc
)/errors
have a readablemessage
property